abci/client/socket_client.go (1)

41-44: Document the ErrClientStopped error variable for clarity.

The addition of the exported variable ErrClientStopped improves error handling consistency. To enhance code readability and maintainability, consider adding a documentation comment explaining the purpose and usage of ErrClientStopped.

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Search for locations where the "client has stopped" error is handled.

# Find all Go files except tests
FILES=$(fd --type f --extension go --exclude '*_test.go')

# Search for direct equality comparisons with error strings
rg -F 'err == "client has stopped"' $FILES

# Search for error comparisons with ErrClientStopped
rg -F 'err == ErrClientStopped' $FILES

# Search for error handling using errors.Is with ErrClientStopped
rg -F 'errors.Is(err, ErrClientStopped)' $FILES

#!/bin/bash
# Let's search for ErrClientStopped definition and usage patterns

# Find where ErrClientStopped is defined
rg -l "ErrClientStopped.*=.*" 

# Search for error handling patterns with ErrClientStopped
rg -A 2 "if.*err.*{" | rg -B 2 "ErrClientStopped"

# Search for error returns with ErrClientStopped
rg "return.*ErrClientStopped"

# Search for error wrapping with ErrClientStopped
rg "fmt.Errorf.*ErrClientStopped"

#!/bin/bash
# Let's examine the ErrClientStopped definition and its usage context

# Show the error definition and surrounding context
rg -B 5 -A 5 "ErrClientStopped.*=.*" abci/client/socket_client.go

# Show all functions that return or use ErrClientStopped
rg -B 10 -A 2 "ErrClientStopped" abci/client/socket_client.go

# Look for any error handling patterns in test files
rg "ErrClientStopped" abci/client/*_test.go

internal/consensus/state_test.go (2)

3286-3286: Unnecessary use of t.Helper() in the test function

The t.Helper() method is intended to be used within helper functions to mark them as helpers. Calling it directly within the test function TestStateTryAddCommitPanicsOnClientError is unnecessary.

Apply this diff to remove the unnecessary call:

-	t.Helper()

---

3350-3352: Verify the panic message to ensure correct behavior

Currently, the test checks for any panic but does not verify if it's the expected one. To ensure the test is validating the correct panic scenario, consider checking the panic value.

Apply this diff to verify the panic message:

-	assert.Panics(t, func() {
+	assert.PanicsWithValue(t, abciclient.ErrClientStopped, func() {
 		_ = consensusState.ctrl.Dispatch(ctx, &TryAddCommitEvent{Commit: commit, PeerID: peerID}, &stateData)
 	})

Configuration used: CodeRabbit UI
Review profile: CHILL

internal/consensus/state_test.go (3)

3278-3354: LGTM! Well structured test for ABCI client error handling

The test effectively verifies that the consensus state panics when encountering an ABCI client error during block processing. The test setup and assertions are clear and comprehensive.

A few suggestions for improvement:

1. Consider adding cleanup of mocks in a defer statement
2. The error message assertion could be extracted to a constant for reuse
3. Consider adding a test case for other ABCI methods like PrepareProposal

 func TestStateTryAddCommitPanicsOnClientError(t *testing.T) {
+    const expectedError = "ABCI client stopped, Tenderdash needs to be restarted: ProcessProposal abci method: client has stopped"
     ctx, cancel := context.WithCancel(context.Background())
     defer cancel()
+    defer func() {
+        app.AssertExpectations(t)
+    }()
     // ... rest of the test
     assert.PanicsWithError(t,
-        "ABCI client stopped, Tenderdash needs to be restarted: ProcessProposal abci method: client has stopped",
+        expectedError,
         func() {

---

3294-3299: Consider enhancing mock error handling

The mock configuration is good but could be enhanced:

1. Add verification that ProcessProposal was actually called
2. Consider testing with different error types beyond ErrClientStopped

 app := clientmocks.NewClient(t)
+app.On("ProcessProposal", mock.Anything, mock.Anything).
-    Return(&abci.ResponseProcessProposal{}, abciclient.ErrClientStopped).
-    Once()
+    Run(func(args mock.Arguments) {
+        // Verify the arguments
+        req := args.Get(1).(*abci.RequestProcessProposal)
+        assert.NotNil(t, req)
+    }).
+    Return(&abci.ResponseProcessProposal{}, abciclient.ErrClientStopped).
+    Once()

---

3309-3323: Consider adding more block validation checks

The block creation and commit handling is solid, but could benefit from additional validation:

1. Validate block metadata matches expected values
2. Add explicit checks for block parts

 block, err := sf.MakeBlock(state, 1, &types.Commit{}, kvstore.ProtocolVersion)
 require.NoError(t, err)
 require.NotZero(t, block.Version.App)
+require.Equal(t, state.LastBlockHeight+1, block.Height)
+require.NotEmpty(t, block.LastBlockID)
 block.CoreChainLockedHeight = 1

 commit, err := factory.MakeCommit(
     ctx,
     block.BlockID(nil),
     block.Height,
     0,
     stateData.Votes.Precommits(0),
     state.Validators,
     privVals,
 )
 require.NoError(t, err)
+require.Equal(t, block.Height, commit.Height)
+require.True(t, commit.BlockID.Equals(block.BlockID(nil)))

Configuration used: CodeRabbit UI
Review profile: CHILL